Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[13.0] Improvements on server.env.techname.mixin #75

Closed
wants to merge 2 commits into from
Closed

[13.0] Improvements on server.env.techname.mixin #75

wants to merge 2 commits into from

Conversation

ivantodorovich
Copy link
Contributor

ping @simahawk

What do you think about this?

Re. the mixin inherit (154090a), I guess it wasn't done like this before because one may use server.env.techname.mixin without server.env.mixing but I can't see why one would want to do that.. I find this way easier to use and in most cases saves a few line breaks

simahawk
simahawk requested changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@simahawk simahawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re. the mixin inherit (154090a), I guess it wasn't done like this before because one may use server.env.techname.mixin without server.env.mixing but I can't see why one would want to do that.. I find this way easier to use and in most cases saves a few line breaks

yeah, it was in my mind for a while. At first I wanted to keep it isolated but it turns out you always use them in combo.

Thanks for taking care of my TODOs! 😄

@simahawk simahawk mentioned this pull request Jul 6, 2021
Merged
@simahawk
Copy link
Contributor

@ivantodorovich could you test this on OCA/search-engine ?

@simahawk
Copy link
Contributor

@ivantodorovich ping :)

@ivantodorovich
Copy link
Contributor Author

@ivantodorovich could you test this on OCA/search-engine ?

It seems ok
Is there something specific you have in mind?

@simahawk
Copy link
Contributor

just use this change there and see if you get a green build ;)

@ivantodorovich
Copy link
Contributor Author

just use this change there and see if you get a green build ;)

Indeed it's not working.

connecto_search_engine would need to be adapted to have the index_prefix_name in a computed field.

But also I tried that, and it's not working as expected. Since server_environment "transforms" fields into computed fields to read from config. I created this issue to explain it: #85

I couldn't find a simple solution for now :/ so best if we close this and reevaluate in a future migration IMO

@simahawk
Copy link
Contributor

@ivantodorovich you can reconsider this here #122 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simahawk simahawk simahawk requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ivantodorovich @simahawk